Partial escape in query string to make search work again#321
Partial escape in query string to make search work again#321tkvogt wants to merge 4 commits intohaskell-github:masterfrom
Conversation
phadej
left a comment
phadej
left a comment
There was a problem hiding this comment.
Looks great, but there are some nitpicks inline.
Orphan instance is a blocker.
| import GitHub.Data | ||
| import GitHub.Internal.Prelude | ||
| import GitHub.Request | ||
| import qualified Network.HTTP.Types as W |
There was a problem hiding this comment.
We use stylish-haskell, to make import pretty put qualified imports into own section:
import Github.Request
import Prelude ()
import qualified Network.HTTP.Types as W| nestedTreeR user repo sha = | ||
| query ["repos", toPathPart user, toPathPart repo, "git", "trees", toPathPart sha] [("recursive", Just "1")] | ||
| query ["repos", toPathPart user, toPathPart repo, "git", "trees", toPathPart sha] | ||
| [("recursive", [W.QE "1"])] |
There was a problem hiding this comment.
4 space indentation (tabular formatting doesn't apply here).
src/GitHub/Data/Request.hs
Outdated
| `hashWithSalt` ps | ||
| `hashWithSalt` body | ||
|
|
||
| instance Hashable W.EscapeItem where |
There was a problem hiding this comment.
orphan instance :/.
Could you make a PR to http-types so the instance is there. Alternatively inline code where instance is needed?
I won't accept PR with orphan instance.
There was a problem hiding this comment.
I should have done this. But now it have wrapped the type in a newtype, because I don't want to go through the whole process of having the right http-types version in stackage.
src/GitHub/Endpoints/Search.hs
Outdated
| -- "q=a+repo:phadej/github&per_page=100" | ||
| -- has to be written as | ||
| -- > searchIssues [("q", [QE "a", QN "+", QE "repo", QN ":", QE "phadej", QN "/", QE "github"]), | ||
| -- ("per_page", [QE "100"])] |
There was a problem hiding this comment.
I'm quite sure this looks ugly when rendered. > have to prefix all code lines.
There was a problem hiding this comment.
In the next version haddock is tested
|
@tkvogt : Thanks for this PR! |
andreasabel
left a comment
andreasabel
left a comment
There was a problem hiding this comment.
Please:
- fix conflicts
- make sure
AllHaskellReposruns
|
This is so long ago. I just remeber that it worked and was good enough for me. It turned into some kind of beauty contest, which I had no time for. |
I understand. Sorry for that. My priorities are rather:
I didn't look closely at 1., but I noticed that 2. is satisfied ( |
|
If you use stack, you can reference this pull request and try it like In my memory it worked. |
Got enough on my plate. I suppose this has to wait for someone with a genuine interest in this functionality to pick it up... |
3 participants
fixes #262
http-types and http-client had to be extended for partial escape. These versions are now in stackage quite a while. I built the structure that I assumed you had in mind (SearchRepoMod), but it is very tedious and in my opinion not worth the trouble. So I used two styles in GitHub.Endpoints.Search